Secrets Manager Proxy #121
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justing-bq
reviewed
Mar 28, 2023
driver/secrets_manager_proxy.cc
Outdated
| } | ||
| } | ||
|
|
||
| bool SECRETS_MANAGER_PROXY::real_connect(const char* host, const char* user, const char* passwd, const char* db, |
There was a problem hiding this comment.
I'm not a fan of overriding the real_connect and real_connect_dns_srv in this way.
Instead can we do the same approach as my PR where we define
bool CONNECTION_PROXY::connect(const char* host, const char* user, const char* password,
const char* database, unsigned int port, const char* socket, unsigned long flags) {
if (ds->enable_dns_srv) {
return this->real_connect_dns_srv(host, user, password, database, flags);
}
return this->real_connect(host, user, password, database, port, socket, flags);
}And then move all of this secrets manager connect logic to
bool SECRETS_MANAGER_PROXY::connect(const char* host, const char* user, const char* password,
const char* database, unsigned int port, const char* socket, unsigned long flags)And in here we can call next_proxy->connect() instead of next_proxy->real_connect().
Look at my PR for reference.
driver/secrets_manager_proxy.cc
Outdated
| auto username = get_from_secret_json_value(USERNAME_KEY); | ||
| auto password = get_from_secret_json_value(PASSWORD_KEY); | ||
| fetched = false; | ||
| auto ret = next_proxy->real_connect(host, username.c_str(), password.c_str(), db, port, unix_socket, clientflag); |
There was a problem hiding this comment.
In general I would prefer to see the actual type rather than the auto keyword (unless the type is super unwieldy)
driver/secrets_manager_proxy.cc
Outdated
| return ret; | ||
| } | ||
|
|
||
| bool SECRETS_MANAGER_PROXY::real_connect_dns_srv(const char* dns_srv_name, const char* user, const char* passwd, |
There was a problem hiding this comment.
Likewise. Another benefit of my earlier suggestion is that we won't need to repeat all this logic again.
driver/secrets_manager_proxy.cc
Outdated
| { | ||
| std::unique_lock<std::mutex> lock(secrets_cache_mutex); | ||
|
|
||
| if (const auto search = secrets_cache.find(this->secret_key); search != secrets_cache.end() && !force_re_fetch) { |
There was a problem hiding this comment.
I'm not really familiar with this if (A; B) notation. Similar to a for loop, I'm guessing the first part is just initialization while the second part is the actual if condition? If so, can we move the const auto search = secrets_cache.find(this->secret_key); to above the if statement? I think it's more clear that way.
| auto res_json = Aws::Utils::Json::JsonValue(json_string); | ||
| if (!res_json.WasParseSuccessful()) { | ||
| MYLOG_DBC_TRACE(dbc, res_json.GetErrorMessage().c_str()); | ||
| throw std::runtime_error("Error parsing secrets manager response body. " + res_json.GetErrorMessage()); |
There was a problem hiding this comment.
Where does this exception get caught and handled?
| else { | ||
| const auto error = "Unable to extract the " + key + " from secrets manager response."; | ||
| MYLOG_DBC_TRACE(dbc, error.c_str()); | ||
| throw std::runtime_error(error); |
There was a problem hiding this comment.
Likewise. Where does this exception get caught?
justing-bq
reviewed
Mar 28, 2023
justing-bq
reviewed
Mar 28, 2023
justing-bq
reviewed
Mar 29, 2023
driver/aws_sdk_helper.h
Outdated
| /** | ||
| * A helper class to initialize/shutdown AWS API once per DLL load/unload. | ||
| */ | ||
| class AWS_SDK_HELPER { |
There was a problem hiding this comment.
With a class this small I'm fine with just defining the entire class in a header file rather than splitting into a .h and .cc.
yanw-bq
force-pushed
the
secrets-manager
branch
from
49bf420
to
f93a67f
Compare
yanw-bq
force-pushed
the
secrets-manager
branch
from
f93a67f
to
4b45ead
Compare
justing-bq
reviewed
Mar 30, 2023
driver/secrets_manager_proxy.cc
Outdated
| if (!secret_ID) { | ||
| const auto error = "Missing required config parameter for Secrets Manager: Secret ID"; | ||
| MYLOG_DBC_TRACE(dbc, error); | ||
| this->set_custom_error_message(error); |
There was a problem hiding this comment.
Have you tested this error?
Doing this works in the connect() logic but I'm less sure of the constructor logic.
justing-bq
approved these changes
Mar 30, 2023
justing-bq
pushed a commit
that referenced
this pull request
Apr 28, 2023
* new secrets_manager files * working secrets manager client * add cache for secrets manager proxy * first unit test for secrets manager proxy * added more unit tests * fix unique lock missing template arguments * fix multiple calls to aws api in secrets manager unit tests * fix multiple aws init/shutdown api calls in integration tests * fix calling init/shutdownApi in windows * verbose log for windows community tests * wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor * move init/shutdown aws api to alloc/free env handle * move init/shutdown aws api to alloc/free env handle * test * test * fix static variables * fix atomic int init * fix unit tests * renaming * cleanup * introduce aws sdk helpoer * address comments * nit * fix rebase * fix rebase * set error * fix build * better error message
yanw-bq
added a commit
that referenced
this pull request
May 2, 2023
* new secrets_manager files * working secrets manager client * add cache for secrets manager proxy * first unit test for secrets manager proxy * added more unit tests * fix unique lock missing template arguments * fix multiple calls to aws api in secrets manager unit tests * fix multiple aws init/shutdown api calls in integration tests * fix calling init/shutdownApi in windows * verbose log for windows community tests * wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor * move init/shutdown aws api to alloc/free env handle * move init/shutdown aws api to alloc/free env handle * test * test * fix static variables * fix atomic int init * fix unit tests * renaming * cleanup * introduce aws sdk helpoer * address comments * nit * fix rebase * fix rebase * set error * fix build * better error message
yanw-bq
added a commit
that referenced
this pull request
May 2, 2023
* new secrets_manager files * working secrets manager client * add cache for secrets manager proxy * first unit test for secrets manager proxy * added more unit tests * fix unique lock missing template arguments * fix multiple calls to aws api in secrets manager unit tests * fix multiple aws init/shutdown api calls in integration tests * fix calling init/shutdownApi in windows * verbose log for windows community tests * wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor * move init/shutdown aws api to alloc/free env handle * move init/shutdown aws api to alloc/free env handle * test * test * fix static variables * fix atomic int init * fix unit tests * renaming * cleanup * introduce aws sdk helpoer * address comments * nit * fix rebase * fix rebase * set error * fix build * better error message
justing-bq
pushed a commit
that referenced
this pull request
May 4, 2023
* new secrets_manager files * working secrets manager client * add cache for secrets manager proxy * first unit test for secrets manager proxy * added more unit tests * fix unique lock missing template arguments * fix multiple calls to aws api in secrets manager unit tests * fix multiple aws init/shutdown api calls in integration tests * fix calling init/shutdownApi in windows * verbose log for windows community tests * wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor * move init/shutdown aws api to alloc/free env handle * move init/shutdown aws api to alloc/free env handle * test * test * fix static variables * fix atomic int init * fix unit tests * renaming * cleanup * introduce aws sdk helpoer * address comments * nit * fix rebase * fix rebase * set error * fix build * better error message
justing-bq
pushed a commit
that referenced
this pull request
May 4, 2023
* new secrets_manager files * working secrets manager client * add cache for secrets manager proxy * first unit test for secrets manager proxy * added more unit tests * fix unique lock missing template arguments * fix multiple calls to aws api in secrets manager unit tests * fix multiple aws init/shutdown api calls in integration tests * fix calling init/shutdownApi in windows * verbose log for windows community tests * wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor * move init/shutdown aws api to alloc/free env handle * move init/shutdown aws api to alloc/free env handle * test * test * fix static variables * fix atomic int init * fix unit tests * renaming * cleanup * introduce aws sdk helpoer * address comments * nit * fix rebase * fix rebase * set error * fix build * better error message
yanw-bq
added a commit
that referenced
this pull request
May 17, 2023
* new secrets_manager files * working secrets manager client * add cache for secrets manager proxy * first unit test for secrets manager proxy * added more unit tests * fix unique lock missing template arguments * fix multiple calls to aws api in secrets manager unit tests * fix multiple aws init/shutdown api calls in integration tests * fix calling init/shutdownApi in windows * verbose log for windows community tests * wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor * move init/shutdown aws api to alloc/free env handle * move init/shutdown aws api to alloc/free env handle * test * test * fix static variables * fix atomic int init * fix unit tests * renaming * cleanup * introduce aws sdk helpoer * address comments * nit * fix rebase * fix rebase * set error * fix build * better error message
alexr-bq
added a commit
that referenced
this pull request
May 18, 2023
* Add AWS Authentication parameters to DSN UI (#115) * Refactor MYSQL_PROXY (#118) * EFM PROXY * complete EFM PROXY * fix unit tests * fix include * generate node keys after setting next proxy for EFM_PROXY * fix unit tests * stop monitoring init() and options() * replace MYSQL_MONITOR_PROXY with MYSQL_PROXY * fix unit tests * stop monitoring ping() * stop monitoring ssl_set and option4 * stop monitoring autocommit * comment out aws code * stop monitoring client_find_plugin * newline at eof * free monitor connection before getting a new one * fix unit tests * test * disable failover and monitoring for monitor thread * fix memory leak in unit test * needs connection handler for monitor * add null check in monitor service start monitoring * fix env handle still used after deleting * comment out mysql_library_init * move connection handler to its own header file * Further refactor MYSQL_PROXY (#119) * create DUMMY_PROXY to handle mysql library calls, refactor MYSQL_PROXY to only forward proxy call * remove commented include * include thread header in dummy proxy * rename mysql_proxy to connection_proxy * rename dummy proxy to mysql proxy * remove secrets manager proxy as it should be a different PR * include in alphabetical order * AWS SDK helper class (#123) * Implement IAM Authentication (#120) * Add Authentication Parameters to ConnectionStringBuilder (#124) * Secrets Manager Proxy (#121) * new secrets_manager files * working secrets manager client * add cache for secrets manager proxy * first unit test for secrets manager proxy * added more unit tests * fix unique lock missing template arguments * fix multiple calls to aws api in secrets manager unit tests * fix multiple aws init/shutdown api calls in integration tests * fix calling init/shutdownApi in windows * verbose log for windows community tests * wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor * move init/shutdown aws api to alloc/free env handle * move init/shutdown aws api to alloc/free env handle * test * test * fix static variables * fix atomic int init * fix unit tests * renaming * cleanup * introduce aws sdk helpoer * address comments * nit * fix rebase * fix rebase * set error * fix build * better error message * Secrets manager integration test (#127) * create secrets in java util * simple sm test * simplify setup * fix integration * comment out toxiporxy * comment out toxiporxy * sm test not inherit from base anymore * fix build * fix set error message, fix dsn not picked up * add {} in connection string * fix curly braces * fix log * fix build * more logging * more logging * fix test * better error handling * uncomment * address comments * revert * IAM Authentication Integration Tests (#126) * Refactor Integration Tests (#128) * Fix installer (#129) * fix minor version exceeding 255, remove lib folder for installer * remove setup lib from installer * add aws sdk component in wix * build aws sdk on release action * workflow dispatch * bundle aws sdk when doing cpack on mac and linux * fix mac installer aws dependencies * Adjust DSN config UI for AWS Authentication (#130) * Documentation for AWS Authentication (#131) * rename windows installer (#132) * Update license (#133) * update license * ignore file change in license * update license in build script * Parse Region from Secret if User does not provide Region (#134) * Fix SQLCancel not going through proxy (#135) * fix SQLCancel not going through proxy * cleanup ds during dbc clone * spacing * Fix typo in doc * Implement ENABLE_STRICT_READER_FAILOVER user parameter (#136) * Override change_user() in IAM/Secrets Manager proxy (#137) * override change_user() in IAM/Secrets Manager proxy * better naming * Fix failover timeout not obeyed (#138) * replace async future with packed_task and thread to avoid blocking future destructor * fix unit tests * add template arguments for std::packaged_task * missing template arguments * use thread pool to run failover and wait for all when driver unloads, fix logger * increase thread pool size * fix reader failover where failed result overwrite successful result * not stop thread pool on free env handle * redirect maven central and stop thread pool in teardown * maven central * add thread id to logs * move future valid check before wait for * move future valid check before wait_for() for writer failover * mark logger as extern * test diable logs * test failover_integration_test first * test * debug gh action * make env var available for debug session * debug in docker * refactor integration tests to use shared pointer for rds client * make rds client back to non static * fix build * reset rds client in teardown * Revert "refactor integration tests to use shared pointer for rds client" This reverts commit 2ec1ae5. * disable remote debug * fix build * debug * specifically call thread pool stop in myodbc_end * move failover thread pool inside env * fix unit test * fix build * cleanup * China endpoint support (#140) * china endpoint support * nit * Verify Writer Cluster Connections (#139) * Fix shadowing member variables causing null pointer exception (#148) * fix some shadowing member variables casuing null pointer exception * extend perf tests aws credentials duration from 3 to 4 hours * workaround mysqlclient * fix zlib not found * fix mac build * bump mysql version * fix mac build * bump github action runner macos versiooon * update download link * fix build * install openssl1.1 for macos13 * debug * debug * fall back to macos 11 * change macos version to 12 * fix build * fix build * Fix monitor timeout (#149) * fix monitor timeout * fix login timeout and unit tests * pass monitor flag * rename a component that may have name collision issue on mac (#151) * Remove is_multi_writer flag and check for last updated writer * Fix tests * Remove uneeded line --------- Co-authored-by: justing-bq <62349012+justing-bq@users.noreply.github.com> Co-authored-by: Yan Wang <68562925+yanw-bq@users.noreply.github.com> Co-authored-by: justing-bq <justing@bitquilltech.com> Co-authored-by: Alex Rehnby-Martin <alexr@AlexRsMacbookPro.local>
yanw-bq
added a commit
that referenced
this pull request
May 18, 2023
* new secrets_manager files * working secrets manager client * add cache for secrets manager proxy * first unit test for secrets manager proxy * added more unit tests * fix unique lock missing template arguments * fix multiple calls to aws api in secrets manager unit tests * fix multiple aws init/shutdown api calls in integration tests * fix calling init/shutdownApi in windows * verbose log for windows community tests * wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor * move init/shutdown aws api to alloc/free env handle * move init/shutdown aws api to alloc/free env handle * test * test * fix static variables * fix atomic int init * fix unit tests * renaming * cleanup * introduce aws sdk helpoer * address comments * nit * fix rebase * fix rebase * set error * fix build * better error message
yanw-bq
added a commit
that referenced
this pull request
May 18, 2023
* Add AWS Authentication parameters to DSN UI (#115) * Refactor MYSQL_PROXY (#118) * EFM PROXY * complete EFM PROXY * fix unit tests * fix include * generate node keys after setting next proxy for EFM_PROXY * fix unit tests * stop monitoring init() and options() * replace MYSQL_MONITOR_PROXY with MYSQL_PROXY * fix unit tests * stop monitoring ping() * stop monitoring ssl_set and option4 * stop monitoring autocommit * comment out aws code * stop monitoring client_find_plugin * newline at eof * free monitor connection before getting a new one * fix unit tests * test * disable failover and monitoring for monitor thread * fix memory leak in unit test * needs connection handler for monitor * add null check in monitor service start monitoring * fix env handle still used after deleting * comment out mysql_library_init * move connection handler to its own header file * Further refactor MYSQL_PROXY (#119) * create DUMMY_PROXY to handle mysql library calls, refactor MYSQL_PROXY to only forward proxy call * remove commented include * include thread header in dummy proxy * rename mysql_proxy to connection_proxy * rename dummy proxy to mysql proxy * remove secrets manager proxy as it should be a different PR * include in alphabetical order * AWS SDK helper class (#123) * Implement IAM Authentication (#120) * Add Authentication Parameters to ConnectionStringBuilder (#124) * Secrets Manager Proxy (#121) * new secrets_manager files * working secrets manager client * add cache for secrets manager proxy * first unit test for secrets manager proxy * added more unit tests * fix unique lock missing template arguments * fix multiple calls to aws api in secrets manager unit tests * fix multiple aws init/shutdown api calls in integration tests * fix calling init/shutdownApi in windows * verbose log for windows community tests * wrap initapi and shutdownapi inside secrets manager proxy contructor/destructor * move init/shutdown aws api to alloc/free env handle * move init/shutdown aws api to alloc/free env handle * test * test * fix static variables * fix atomic int init * fix unit tests * renaming * cleanup * introduce aws sdk helpoer * address comments * nit * fix rebase * fix rebase * set error * fix build * better error message * Secrets manager integration test (#127) * create secrets in java util * simple sm test * simplify setup * fix integration * comment out toxiporxy * comment out toxiporxy * sm test not inherit from base anymore * fix build * fix set error message, fix dsn not picked up * add {} in connection string * fix curly braces * fix log * fix build * more logging * more logging * fix test * better error handling * uncomment * address comments * revert * IAM Authentication Integration Tests (#126) * Refactor Integration Tests (#128) * Fix installer (#129) * fix minor version exceeding 255, remove lib folder for installer * remove setup lib from installer * add aws sdk component in wix * build aws sdk on release action * workflow dispatch * bundle aws sdk when doing cpack on mac and linux * fix mac installer aws dependencies * Adjust DSN config UI for AWS Authentication (#130) * Documentation for AWS Authentication (#131) * rename windows installer (#132) * Update license (#133) * update license * ignore file change in license * update license in build script * Parse Region from Secret if User does not provide Region (#134) * Fix SQLCancel not going through proxy (#135) * fix SQLCancel not going through proxy * cleanup ds during dbc clone * spacing * Fix typo in doc * Implement ENABLE_STRICT_READER_FAILOVER user parameter (#136) * Override change_user() in IAM/Secrets Manager proxy (#137) * override change_user() in IAM/Secrets Manager proxy * better naming * Fix failover timeout not obeyed (#138) * replace async future with packed_task and thread to avoid blocking future destructor * fix unit tests * add template arguments for std::packaged_task * missing template arguments * use thread pool to run failover and wait for all when driver unloads, fix logger * increase thread pool size * fix reader failover where failed result overwrite successful result * not stop thread pool on free env handle * redirect maven central and stop thread pool in teardown * maven central * add thread id to logs * move future valid check before wait for * move future valid check before wait_for() for writer failover * mark logger as extern * test diable logs * test failover_integration_test first * test * debug gh action * make env var available for debug session * debug in docker * refactor integration tests to use shared pointer for rds client * make rds client back to non static * fix build * reset rds client in teardown * Revert "refactor integration tests to use shared pointer for rds client" This reverts commit 2ec1ae5. * disable remote debug * fix build * debug * specifically call thread pool stop in myodbc_end * move failover thread pool inside env * fix unit test * fix build * cleanup * China endpoint support (#140) * china endpoint support * nit * Verify Writer Cluster Connections (#139) * Fix shadowing member variables causing null pointer exception (#148) * fix some shadowing member variables casuing null pointer exception * extend perf tests aws credentials duration from 3 to 4 hours * workaround mysqlclient * fix zlib not found * fix mac build * bump mysql version * fix mac build * bump github action runner macos versiooon * update download link * fix build * install openssl1.1 for macos13 * debug * debug * fall back to macos 11 * change macos version to 12 * fix build * fix build * Fix monitor timeout (#149) * fix monitor timeout * fix login timeout and unit tests * pass monitor flag * rename a component that may have name collision issue on mac (#151) * Remove is_multi_writer flag and check for last updated writer * Fix tests * Remove uneeded line --------- Co-authored-by: justing-bq <62349012+justing-bq@users.noreply.github.com> Co-authored-by: Yan Wang <68562925+yanw-bq@users.noreply.github.com> Co-authored-by: justing-bq <justing@bitquilltech.com> Co-authored-by: Alex Rehnby-Martin <alexr@AlexRsMacbookPro.local>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Review Status
Summary
Implement Secrets Manager Proxy
Description
SECRETS_MANAGER_PROXYAdditional Reviewers
@justing-bq @sergiyvamz